Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix exceeded the prepaid gas when upgrade remote contract #197

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fospring
Copy link

No description provided.

@fospring
Copy link
Author

fospring commented Jan 27, 2023

When GAS_FOR_UPGRADE_REMOTE_DEPLOY is 15T, upgrade remote contract will throw error of Exceeded the prepaid gas. Updated GAS_FOR_UPGRADE_REMOTE_DEPLOY to 25T would throw same error, So I updated it to 30T.

This is the failed tx when GAS_FOR_UPGRADE_REMOTE_DEPLOY is 25T: https://explorer.testnet.near.org/transactions/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m

Copy link

@agileurbanite agileurbanite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I don't see this negatively impacting other DAOs

@agileurbanite
Copy link

But I believe you'll need @ilblackdragon approval given that he's the main contributor to sputnik atm.

@fospring
Copy link
Author

fospring commented Feb 2, 2023

@ilblackdragon can you pls review this MR?

@ilblackdragon
Copy link
Collaborator

ilblackdragon commented Feb 4, 2023

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via near/NEPs#264 that allows to not have this at all, at it calculates leftover gas after function already completed: near/near-sdk-rs#740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

@fospring
Copy link
Author

fospring commented Feb 6, 2023

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via near/NEPs#264 that allows to not have this at all, at it calculates leftover gas after function already completed: near/near-sdk-rs#740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

@fospring fospring closed this Feb 6, 2023
@fospring fospring reopened this Feb 6, 2023
@fospring
Copy link
Author

fospring commented Feb 6, 2023

I'm having trouble opening the execution plan for this transaction (https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#execution)

Mostly question is why this is needed, because this change may be hiding another issue. LEFTOVER parameters are to cover costs of calling promise_function_call itself and returning Promise and it should be not be taking this much gas. It shouldn't even be 15Tg in the first place.

There is also new API in 4.0 SDK via near/NEPs#264 that allows to not have this at all, at it calculates leftover gas after function already completed: near/near-sdk-rs#740

If we are updating this, let's switch to this API given sputnikdao2 is already on 4.0 sdk.

May be there are some trouble for FE in query execution plan:
https://testnet.nearblocks.io/txns/AR5j8Dz3Ts4ZDKXr1Zd5Eq7VGeS23AmWDtpjhih2wq2m#
image

@agileurbanite
Copy link

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github:
https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it:
https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in:
#198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@fospring
Copy link
Author

fospring commented Feb 6, 2023

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon My executions flows:

  1. create mock accounts:
  1. build dao :
git clone [email protected]:near-daos/sputnik-dao-contract.git
cargo build --release -p sputnikdao2 --target wasm32-unknown-unknown
  1. deploy dao contract(or1007owner-2.yongchun.testnet, owner of or1007-2.yongchun.testnet) by this transaction
  2. deploy smart contract to or1007-2.yongchun.testnet with owner or1007owner-2.yongchun.testnet by this transaction
  3. store-blob of smart contract, ready to upgrade smart contract of or1007-2.yongchun.testnet by this transaction, the store-blob's base58_crypto hash is Hy8JJwzpRvhnSnEoQnF1WvDU5kZFksAbiScXsDeuzLZg
  4. add proposal of remote upgrade or1007-2.yongchun.testnet smart contract, and act proposal:

I think I have tested that 15T gas LEFTOVER is not enough for action of UpgradeRemote.

@fospring
Copy link
Author

fospring commented Feb 6, 2023

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon, Thank you for your reminder. I have added a new transaction test that when FACTORY_UPDATE_GAS_LEFTOVER is 15Tg, it's enough, so I reverted updation of FACTORY_UPDATE_GAS_LEFTOVER by this commit e2ab7ad036c0a842fd69ba05f7bceb12cc740f84

pls check again

@fospring
Copy link
Author

fospring commented Feb 14, 2023

We need to migrate sputnikdao2 to the latest sdk as it's not even on 4.0 it's currently on 4.0.0-pre.4 which is not even defined as a release version on github: https://github.com/near/near-sdk-rs/releases?expanded=true&page=2&q=4.0.0-pre.4

But strangely there are rust docs on it: https://docs.rs/near-sdk/4.0.0-pre.4/near_sdk/index.html

Either way, I'm not sure near/NEPs#264 was even introduced in 4.0.0-pre.5 as it looks like it was pretty of the changes for 4.0.0 stable release here: https://github.com/near/near-sdk-rs/releases/tag/4.0.0

What does this mean?

Well to migrate sputnikdao2 to >= 4.0.0 a few things need to happen:

  1. Migrate to the new rust SDK version and update deprecated methods
  2. Remove near-sdk-sim and migrate to workspaces-rs

The above is covered in: #198

Overall, however, my perception of the task is that it seems like a larger effort.

@ilblackdragon do you have concerns with increasing FACTORY_UPDATE_GAS_LEFTOVER as a stop gap for now and then during the follow up with updating the SDK we can re-evaluate the need for this as part of that effort.

Also @fospring to @ilblackdragon point, I do believe you need to re-evaluate your implementation as it should not take 15Tg in the first place. Can you share pseudo code of your implementation or your execution plan in detail?

@agileurbanite @ilblackdragon can we hotfix on this issue? https://explorer.testnet.near.org/transactions/3xRbdcbdQnC1jSo2arzzQ6bJDNs7PPdrLARmvYFbRL68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants